Conversation
…actor - Track selfDestructedAt access index so balance writes from later txns are not incorrectly skipped in BAL computation - Fix net-zero storage write detection in BAL - Extract BlockAssembler from builderstages/exec.go into exec package - Add merge.NewFaker and Prague pre-deploy support in exec module tester - Fix EIP-7702 committed flag for BAL tracking in state_object - Integrate versionedStateReader and parallel exec determinism fixes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reorder AccountPath enum so SelfDestructPath precedes BalancePath, ensuring updateWrite always sets the selfDestructed flag before evaluating balance skip logic. Add sortVersionedWrites to AsBlockAccessList to guarantee deterministic write processing order regardless of Go map iteration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR reinstates the BlockAssembler abstraction and consolidates Block Access List (BAL / EIP-7928) generation/normalization into the versioned I/O pipeline, while applying a set of fixes for determinism and fork-specific behavior (Amsterdam/Prague/Cancun) across execution, block building, and engine API surfaces.
Changes:
- Reintroduce a reusable
BlockAssemblerand route BAL capture/computation throughstate.VersionedIO.AsBlockAccessList()rather than stagedsync helper code. - Fix/adjust BAL encoding/decoding, determinism (sorted writes), and selfdestruct/net-zero semantics in versioned execution paths.
- Update block/test generation and Engine API plumbing to support newer fork requirements (e.g., Cancun beacon root availability, transition configuration exchange).
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| node/ethconfig/config.go | Adds default ExperimentalBAL config value. |
| node/cli/flags.go | Sets explicit default value for --experimental.bal. |
| execution/types/block_access_list_test.go | Clarifies RLP encoding expectations in test comments. |
| execution/types/block_access_list.go | Switches BAL RLP to minimal integer encoding + matching decode helpers. |
| execution/tests/blockgen/chain_makers.go | Records system-call I/O for BAL and sets Cancun beacon root prior to init. |
| execution/state/versionmap.go | Reorders AccountPath enum to ensure deterministic/selfdestruct-correct processing. |
| execution/state/versionedio.go | Adds deterministic write sorting and improves BAL construction rules (net-zero, selfdestruct). |
| execution/state/state_object.go | Adjusts committed-write tracking for versioned reads when setting storage. |
| execution/state/journal.go | Preserves read-set entries when reverting a touch to keep validation correct. |
| execution/state/intra_block_state_test.go | Updates test to clear stale caches before simulating re-execution. |
| execution/state/intra_block_state.go | Fixes versioned read tracking behavior and codehash sync during versionmap code reads. |
| execution/stagedsync/exec3_serial.go | Removes/adjusts Amsterdam+serial-exec messaging and minor whitespace cleanup. |
| execution/stagedsync/exec3_parallel.go | Adds BAL mismatch dumping and changes BAL mismatch handling behavior. |
| execution/stagedsync/bal_create.go | Simplifies BAL creation to use VersionedIO.AsBlockAccessList(). |
| execution/protocol/state_transition.go | Names state refund intermediate for clarity. |
| execution/protocol/block_exec.go | Expands gas-used mismatch logging with per-tx diagnostics. |
| execution/execmodule/execmoduletester/exec_module_tester.go | Sets default experimentalBAL for module tester options. |
| execution/execmodule/block_building.go | Only bumps payload version / includes BAL fields when header carries BAL hash. |
| execution/exec/txtask.go | Avoids genesis conversion when genesis == nil. |
| execution/exec/block_assembler.go | Reintroduces BlockAssembler with BAL capture during init/tx/finalize. |
| execution/engineapi/interface.go | Extends EngineAPI interface with transition configuration exchange method. |
| execution/engineapi/engine_server_test.go | Updates tests to assert BAL presence rather than exact “empty BAL” encoding. |
| execution/engineapi/engine_server.go | Returns UnknownPayload when assembled response has nil ExecutionPayload. |
| execution/engineapi/engine_api_methods.go | Implements ExchangeTransitionConfigurationV1. |
| execution/builder/builderstages/finish.go | Only embeds BAL hash into header for Amsterdam+ blocks. |
| execution/builder/builderstages/exec.go | Switches builder execution to use BlockAssembler and re-enables forced parallel on Amsterdam. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TEMPORARY: warn instead of error to allow sync to continue for debugging | ||
| log.Warn("BAL mismatch (continuing)", "block", applyResult.BlockNum, "computed", bal.Hash(), "expected", headerBALHash, "storedBAL", dbBALBytes != nil) | ||
| } |
There was a problem hiding this comment.
Continuing sync when the computed BAL hash doesn’t match the header hash breaks block validity rules for Amsterdam blocks. This should return an error (and mark the block invalid) unless explicitly gated behind a dedicated debug/devnet flag; otherwise the node may accept/serve an invalid chain state.
execution/exec/block_assembler.go
Outdated
| func (ba *BlockAssembler) Initialize(ibs *state.IntraBlockState, tx kv.TemporalTx, logger log.Logger) { | ||
| protocol.InitializeBlockExecution(ba.cfg.Engine, | ||
| NewChainReader(ba.cfg.ChainConfig, tx, ba.cfg.BlockReader, logger), ba.Header, ba.cfg.ChainConfig, ibs, &state.NoopWriter{}, logger, nil) | ||
| if ba.HasBAL() { | ||
| ba.balIO = ba.balIO.Merge(ibs.TxIO()) | ||
| ibs.ResetVersionedIO() | ||
| } |
There was a problem hiding this comment.
BlockAssembler.Initialize calls protocol.InitializeBlockExecution (which returns an error) but ignores the return value. Initialization failures (e.g., system contract init/predeploy issues) would be silently swallowed and could produce invalid blocks/state. Change Initialize to return an error and propagate it to callers (e.g., SpawnBuilderExecStage) so block building fails fast on init errors.
| // Set ParentBeaconBlockRoot for Cancun+ blocks before InitializeBlockExecution | ||
| // so that EIP-4788 can store it during initialization. | ||
| if config.IsCancun(b.header.Time) { | ||
| var beaconBlockRoot common.Hash | ||
| if _, err := rand.Read(beaconBlockRoot[:]); err != nil { | ||
| return nil, nil, nil, fmt.Errorf("can't create beacon block root: %w", err) | ||
| } | ||
| b.header.ParentBeaconBlockRoot = &beaconBlockRoot | ||
| } |
There was a problem hiding this comment.
ParentBeaconBlockRoot is being set using crypto/rand, which makes GenerateChain non-deterministic and can cause flaky tests. Also, this file already sets ParentBeaconBlockRoot again later in block generation; adding a value here risks it being overwritten after EIP-4788 initialization, making the header inconsistent with the state. Suggest deriving a deterministic root (e.g., from block number/parent hash) and ensuring it’s assigned exactly once per block (before InitializeBlockExecution).
There was a problem hiding this comment.
I don't agree with this reasoning this makes a deterministic link for non deterministic data - this causes assumed determinism bugs. The double assigment is a bug, which should be tested and reviewed.
There was a problem hiding this comment.
The duplication was an error its fixed by: 3628943
| return nil, err | ||
| } | ||
|
|
||
| func (e *EngineServer) ExchangeTransitionConfigurationV1(ctx context.Context, beaconConfig *engine_types.TransitionConfiguration) (*engine_types.TransitionConfiguration, error) { |
There was a problem hiding this comment.
exchangeTransitionConfiguration was removed a long time ago (see #12430). Why restore it?
There was a problem hiding this comment.
It seems to cause test fails when hive testing - I'll investigate the issue.
There was a problem hiding this comment.
Fixed by: b07260a the testing agent was using an out of data hive instance.
| @@ -0,0 +1,164 @@ | |||
| # Launch bal-devnet-2 (Erigon + Lighthouse) | |||
There was a problem hiding this comment.
this looks like a duplicated file.... a commit adds this to main already - 7c13b94
There was a problem hiding this comment.
Fixed by: 6b7d81a
Claude adds 'skills.md' files locally while making skills changes, these should never be checked into ci, so I've added a .gitignore entry
The PR reinstates the BlockAssembler and the move of BAL processing to the versionedio code rather than as an auxiliary set of functions in the sync process. The main advantage of this change is that it consolidates behaviour which makes agent understanding more focussed and makes the bal code re-usable in tests. This makes the codebase more streamlined avoids breaks and confusion in test analysis. It contains fixes merged by the claude agent from #19434 and #19525, along with fixes that get to a running bal-devnet-2 node. This is ready for merging. --------- Co-authored-by: taratorio <94537774+taratorio@users.noreply.github.com> Co-authored-by: yperbasis <andrey.ashikhmin@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Andrew Ashikhmin <34320705+yperbasis@users.noreply.github.com>
…fter post block validation and before commitment validation (#19595) DO NOT MERGE BEFORE #19528 - adds the new bal-devnet-3 test fixtures and applies the skips for failing eips - moves the bals hash check after post block validation and before commitment verification so that we can clearly see the real errors (e.g. gas mismatches/receipt mismatches/etc) instead of always seeing `block access list mismatch`
The PR reinstates the BlockAssembler and the move of BAL processing to the versionedio code rather than as an auxiliary set of functions in the sync process. The main advantage of this change is that it consolidates behaviour which makes agent understanding more focussed and makes the bal code re-usable in tests. This makes the codebase more streamlined avoids breaks and confusion in test analysis. It contains fixes merged by the claude agent from #19434 and #19525, along with fixes that get to a running bal-devnet-2 node. This is ready for merging. --------- Co-authored-by: taratorio <94537774+taratorio@users.noreply.github.com> Co-authored-by: yperbasis <andrey.ashikhmin@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Andrew Ashikhmin <34320705+yperbasis@users.noreply.github.com>
…fter post block validation and before commitment validation (#19595) DO NOT MERGE BEFORE #19528 - adds the new bal-devnet-3 test fixtures and applies the skips for failing eips - moves the bals hash check after post block validation and before commitment verification so that we can clearly see the real errors (e.g. gas mismatches/receipt mismatches/etc) instead of always seeing `block access list mismatch`
The PR reinstates the BlockAssembler and the move of BAL processing to the versionedio code rather than as an auxiliary set of functions in the sync process.
The main advantage of this change is that it consolidates behaviour which makes agent understanding more focussed and makes the bal code re-usable in tests. This makes the codebase more streamlined avoids breaks and confusion in test analysis.
It contains fixes merged by the claude agent from #19434 and #19525, along with fixes that get to a running bal-devnet-2 node.
This is ready for merging.